Pull request overview

This PR refactors the E2E harness to use a single shared per-location VNet and Bastion (in abe2e-{location}) and attaches all AKS clusters to per-cluster subnets inside that VNet, reducing repeated Bastion provisioning and centralizing shared infrastructure creation.

Changes:

• Add shared-infrastructure provisioning (shared_infra.go) to create/ensure a shared VNet, Bastion, Firewall, and managed identity, plus auto-allocation of per-cluster subnets.
• Update cluster creation and networking helpers to rely on VnetSubnetID (BYO VNet/subnet) rather than discovering VNets in the MC_ resource group.
• Update SSH-over-Bastion flow and documentation to reflect shared Bastion/VNet usage.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Nice consolidation — moving to a single per-region shared VNet/Bastion/Firewall is a big simplification and should cut Bastion cost significantly. Most of my comments are about correctness under concurrent e2e processes in the same region, since the new shared resources are by design global within a region and the existing in-process cachedFunc does not protect against another go test invocation racing on the same ARM resources.

🔴 High — cleanupOrphanedSubnets can delete an in-flight subnet of a concurrent run

e2e/shared_infra.go ensureSharedInfra → cleanupOrphanedSubnets

The deletion criterion is "no active resources on the subnet AND the AKS cluster doesn't exist (404)". But in prepareCluster we create the subnet before getOrCreateCluster. There is a real window where:

• process A is creating aks-subnet-<cluster> → subnet exists, IPConfigurations empty, AKS Get returns 404 (cluster not yet PUT)
• process B starts cold, runs ensureSharedInfra → cleanupOrphanedSubnets → sees A's subnet as orphaned and deletes it

This would surface as a confusing failure where A's cluster creation later can't find its subnet. Suggest one of:

• Only delete subnets whose provisioningState=Succeeded and whose age (from a tag we set at creation, e.g., createdAt) is older than some safety threshold (e.g., 1h).
• Or skip cleanup entirely and rely on the abe2e-{location} RG TTL/janitor.

🔴 High — ensureClusterSubnet retries the same CIDR on 409

cidr := allocateSubnetCIDR(subnetName, usedCIDRs)
...
return ensureSubnet(ctx, rg, SharedVNetName, subnetName, cidr) // wraps POST in retryOn409

CIDR allocation is outside the retry. If two processes hash-collide (or merely list usedCIDRs before the other commits), both pick the same /20 and POST. The loser gets 409, retryOn409 re-POSTs the same CIDR → 409 forever until the 10-attempt budget runs out. With backoff that's a ~50s hang for nothing.

Move the allocate+list inside the retry function, e.g.:

return retryOn409(ctx, "allocating cluster subnet", func() error {
    used, err := listUsedCIDRs(ctx, rg)
    if err != nil { return err }
    cidr := allocateSubnetCIDR(subnetName, used)
    if cidr == "" { return fmt.Errorf("no free /20") }
    return createSubnet(ctx, rg, vnet, subnetName, cidr)
})

Also: usedCIDRs is built with an exact string match on AddressPrefix. A subnet that's a /24 (or /26, like AzureBastionSubnet at 10.0.0.0/26) won't be in the set as /20, so overlap is not detected by allocateSubnetCIDR itself. Today this is safe because shared subnets all live in 10.0.0.0/16 and cluster subnets start at 10.1.0.0/20, but it's a footgun if anyone ever lowers the second-octet start. Worth a comment, or compute overlap properly with net.ParseCIDR.

🟡 Medium — ensureSharedBastion / ensureSharedFirewall / ensureClusterIdentity not idempotent across processes

These do Get → 404 → POST. cachedFunc only dedupes within one process. Two cold parallel go test invocations in the same region will both POST a new Bastion / Firewall / UAMI / role assignment. Bastion and Firewall creates are not idempotent on conflict and are also slow (5-10min), so the loser will hit either a 409 or a long timeout with no retry.

Either:

• wrap them in retryOn409 with a Get on each retry (so the second attempt finds the now-existing resource and returns), or
• gate them with a subscription-scoped lease (e.g., a resource group lock or a "create-claim" tag).

ensureClusterIdentity already swallows the 409 on the role assignment — same pattern needed on the UAMI create itself.

🟡 Medium — setupPrivateDNSForAPIServer now runs for network-isolated clusters too

In the old code this was only called for non-isolated clusters. The diff removes that guard implicitly (it's unconditional in the new prepareCluster DAG). For a private/network-isolated cluster, AKS RP already provisions its own private DNS for the API server FQDN in the MC_ RG. Adding a second A record in our shared hcp.<location>.azmk8s.io zone may either be harmless (different zone name from the AKS-managed one) or conflict with resolution.

Was this intentional? If yes, please add a one-line comment explaining why both records can coexist. If not, restore the if !isNetworkIsolated guard.

🟡 Medium — addRecordSetToPrivateDNSZone silently produces wrong record names when FQDN doesn't end in .azurecr.io

recordName := strings.TrimSuffix(*fqdn, ".azurecr.io")

TrimSuffix is a no-op when the suffix is absent → recordName == fqdn, and we then write an A record with a fully-qualified name as a relative label, which Azure will either reject (4xx → bubbles up) or — worse — accept as <fqdn>.privatelink.azurecr.io. Suggest:

recordName, ok := strings.CutSuffix(*fqdn, ".azurecr.io")
if !ok {
    return fmt.Errorf("PE NIC FQDN %q does not match expected .azurecr.io suffix", *fqdn)
}

🟢 Low — migration impact of cluster name version bump

cache.go bumps cluster names (v3→v4, v4→v5, v1→v2). Existing CI/dev environments still have old MC_ RGs and per-cluster bastions/PIPs/firewalls that won't be deleted by cleanupOrphanedSubnets (it only scans the shared VNet subnets). Worth either:

• documenting a one-time az group delete cleanup step in e2e/README.md, or
• adding a cleanupOrphanedClusterRGs pass that deletes MC_abe2e-*-{location} RGs whose cluster no longer exists.

Otherwise leftover bastions ($$$) and PIPs will linger in every region indefinitely.

🟢 Low — setupPrivateDNSForAPIServer DAG dep is implicit via MustGet

dag.Run(g, func(ctx context.Context) error {
    return setupPrivateDNSForAPIServer(ctx, vNet.MustGet().resourceGroup, cluster)
})

This works (MustGet blocks on <-r.done), but it's a hidden dependency on the vNet task. If vNet fails, MustGet panics → the recover-as-error in dag catches it, but we get a misleading panic-stack error rather than a clean cascaded cancellation. Prefer dag.Run1(g, vNet, func(ctx, v) error { return setupPrivateDNSForAPIServer(ctx, v.resourceGroup, cluster) }) to match the rest of the function's style.

The architectural direction (single shared VNet, one Bastion, shared identity, hash-allocated cluster subnets, single DNS zone with per-cluster A records) is good. Main blockers IMO are the two High items above; the rest are improvements.